OPFS VFSs: add getRoot and prefix options (upstream #380) - #95

Thanks for the review, @rkistner.

Link. You're right, MDN has no page for Storage Buckets. I've replaced the link with the spec, https://wicg.github.io/storage-buckets/, in the description and in the code comment.

Browser support. The API is shipped and on by default in Chromium since version 122, in Chrome, Edge, Opera and Chrome for Android (Chrome's announcement). caniuse shows it supported for 72.15% of global browser usage, with no Firefox or Safari support, and MDN's compatibility data lists it as experimental and standards-track. That is why this is opt-in and feature-detected: without the option nothing changes, in any browser. The defect is in Chromium's file system code, and the API exists in the same Chromium-based browsers.

A Chrome bug. Agreed, the cause is in Chromium and it should be fixed there. We have a report for Chromium ready with these steps, and I'll link it here once it's filed. A fix there will take a while to reach installed browsers, though, and until then an app can't avoid it from the outside.

How users run into it. The DevTools button doesn't trigger it, as you say: it keeps the storage bucket. The everyday ways of clearing a site's data do. Measured today on Chrome 154.0.8037.98, with a tab of the site open while the data is cleared:

I've added the steps for all three to the description. We see it in production on Chrome and Edge, on macOS and Windows. It affects a few users a month, but for each of them the app can't open its database until they restart the browser.

Reloading doesn't help. Once it happens, reloading the page and opening new tabs fail the same way. It ends only when the browser restarts, or after 10 minutes in which no site in that browser profile uses OPFS. Every retry starts those 10 minutes again. So a better error would have to ask the user to restart the browser, not to reload.

Detection. It can be detected: listing the root directory fails with this InvalidStateError text twice in a row, about 3 seconds per call. We've written that check for our app so it can show the restart instruction, and I'm happy to add a similar check to the SDK, so it throws a specific error instead of a generic open failure. Detection can't help with the second effect, though: when a newer tab's database is deleted, that tab gets no error at all.

Other workarounds we tried.

• Removing the folder through a handle from before the deletion ends the fault. But the same call deletes everything when storage is working, and only a tab that still holds such a handle can do it.
• Stopping the database the moment the page receives the storage event for the cleared localStorage avoided the fault in our tests. But it relies on an order Chrome doesn't promise, and the same event fires when another tab clears localStorage.

The named bucket is the only approach that avoided both effects in every case we tried.

Suggestion. Keep this as an opt-in option, so apps that don't set it see no change. If you'd rather the library didn't refer to Storage Buckets at all, the VFS could instead take a function that returns its root directory, plus a prefix for its lock names, and the bucket would be entirely the app's choice. Separately, I can open a PR for the detection with a specific error, if that's useful to you.

I agree that we shouldn't ship workarounds for things that should be fixed in Chrome (but I'm not sure if this continuing to work for named buckets is intentional, these not being affected when site data is cleared sounds like named buckets just aren't cleaned up properly).

That said, given that OPFS writeahead only works on Chrome too (and uses flags that haven't made it into the standard yet), I'm not opposed to adding support for storage buckets for the other isolation features they bring.

However, instead of this package probing navigator.storageBuckets, what if we instead added an option to the VFS (say getRoot: () => Promise<FileSystemDirectoryHandle>, and a lock prefix for the writeahead vfs) and kept that logic outside of wa-sqlite (probably in PowerSync then)? This would make it easier to upstream and is more flexible (also supports using a custom child directory in the default bucket as root).

@darshanp40 Thanks for the extra details. I missed in your original report that the user can easily go to site settings to delete the data, without manually navigating to chrome://settings/content/siteDetails. I also missed that you need a full browser restart and not just a tab reload - that makes the impact significantly worse.

@simolus3 I like your suggestion of accepting an FileSystemDirectoryHandle (or callback returning it). In theory that could also support "unpartitioned" OPFS roots via the Storage Access API, and potentially other storage locations in the future if browsers add support.

With that we'd still have this broken for users not using storage buckets, but at least there would be a workaround available until the issue is fixed in Chrome.

Thanks, both. I've reworked the PR along those lines.

The API. The VFSs no longer know about Storage Buckets. AccessHandlePoolVFS, OPFSCoopSyncVFS and OPFSWriteAheadVFS take getRoot: () => Promise<FileSystemDirectoryHandle>, with () => navigator.storage.getDirectory() as the default. OPFSCoopSyncVFS and OPFSWriteAheadVFS call it again on every open; AccessHandlePoolVFS calls it once, when it starts. OPFSCoopSyncVFS and OPFSWriteAheadVFS also take lockPrefix, put in front of every Web Lock and BroadcastChannel name they derive from a file or directory name: #read, #write, #open, #ckpt, #wa, -txId<…> and the .session-* directory locks, and the ahp: channels and .ahp-* directory locks. Without the options nothing changes, including the names, so tabs on an earlier version still coordinate with new ones. A function rather than a handle, because the two VFSs that reopen files ask for the root again on every open: a handle obtained before the site's data was deleted would point at the old bucket, while the function returns a current one and creates a deleted bucket again. The bucket logic moved to powersync-ja/powersync-js#1142, which maps its storageBucket option to these two. A directory below the default root works the same way, and so does any other origin private file system directory the function returns, for example one reached through the Storage Access API.

Tests. The suite now uses a directory below the default root instead of a bucket, so it runs in every browser the suite runs in: the files land only under that root, same-named files under the two roots keep their own data and never wait on each other, and a getRoot that fails fails the open with its error. The vfs_xAccess, vfs_xOpen, vfs_xClose, vfs_xRead and vfs_xWrite suites run under the custom root as well. 15 files, 6,287 tests pass on Chrome 154.0.8037.98. With the prefix emptied, the four same-name tests of OPFSCoopSyncVFS and OPFSWriteAheadVFS fail, OPFSWriteAheadVFS with invalid WAL file, so the prefix is still needed.

On named buckets not being cleaned up (@simolus3). I checked, since it would matter. They are deleted with the rest of the site's data. Measured on Chrome 154.0.8037.98 with a bucket app-db that held a file before the deletion:

The same with no tab open during the deletion: the bucket and its file are gone. In the default bucket the listing through the old handle also fails at once. The create through it is where the two differ: it fails after about 6 seconds, with the same InvalidStateError, and deletes the new tab's file. The source agrees: in content/browser/storage_partition_impl.cc, clearing the quota-managed data lists the buckets modified in the time range with GetBucketsModifiedBetween, keeps those of the storage key and calls DeleteBucketData for each. So named buckets are cleared like the default one. The difference is where the files live: a named bucket's directory is per bucket id (GetClientBucketPath), so a deleted bucket's handles point at a directory that no longer exists, while the default bucket's directory is chosen by origin and the recreated bucket reuses it. I've put this in the description.

@rkistner, agreed that apps on the default root stay exposed until Chromium fixes this. The option gives them a way out that did not exist before, and the description now leads with the generic use of the two options, with the Chromium material at the end.

Chromium report. Still to be filed on our side; I'll link it here when it is.

Hi @rkistner and @simolus3, a gentle follow-up on this one.

The PR now follows the direction you suggested on 7 October: the VFSs take a getRoot function and a lockPrefix, with no Storage Buckets code in the library, and the bucket logic lives in powersync-ja/powersync-js#1142. Without the new options, behaviour is unchanged.

Could you let me know:

1. Whether this API matches what you had in mind, or whether you would like changes before a full review.
2. Whether you could approve the workflow runs for this PR. They are waiting for maintainer approval, so CI has not run yet. The full suite passes locally (15 files, 6,287 tests on Chrome 154).
3. If it is accepted, roughly which release it could go into. I noticed 2.0.7 went out on 12 October, and #1142 will move its catalog pin to the release that includes this change.

I am happy to make any changes needed. Thank you for your time.

Thanks for the reminder, I have approved the CI run.

I'm happy with the API changes here, they're pretty simple and should work for other use-cases as well. Given that this is also relevant for other (non-PowerSync) wa-sqlite users, do you think you could file it against rhashimoto/wa-sqlite as well? If Roy has concerns about it we can always consider landing it here first, but ideally we don't want too many changes in our fork apart from linking the PowerSync core extension.

Thanks @simolus3, for approving the run and for the review.

I've opened the same change against the original project as rhashimoto#380. All the files it touches are identical there, and it applies to the current master without changes. The full suite passes on that branch: 15 files, 6,443 tests. The .changeset file is the only part left out, since that project does not use changesets.

I'll keep this PR open until Roy has had a look. If he accepts it there, this one can be closed and the change will reach this fork with the next merge from upstream. If he would rather not take it, I'm happy to land it here as you suggested.

I'll open a PR to merge from upstream, which now includes your changes. Thanks!